[EuiPopover and parts] Converted to Emotion #5977

cchaos · 2022-06-16T18:20:56Z

[Breaking] EuiPopover's `display` prop now accepts real CSS `display` values

The reason this is breaking is because the old value inlineBlock needed to change to snake case inline-block. This shouldn't be a big deal since inline-block is default.

[New] EuiPopoverPanel (internal only)

Some other EUI components were duplicating the .euiPopover__panel classes on EuiPanel instances to recreate the popover styles. Instead, I've pulled that EuiPanel out of EuiPopover directly and created EuiPopovePanel that other components like EuiComboBox now use. It mainly handles the styling based on position, isOpen, or isAttached.

Added `EuiPopoverPanelContext` to pass through `panelPadding`

The popover title and footer components used a complicated set of parent/child selectors to inherit padding and/or create margins to account for panel padding. Instead of sticking with the nested selectors, I opted for creating a context that the EuiPopoverTitle and Footer components used to decide how it should handle those margins and it's own padding size.

This is debatable whether context here is really necessary, so I'm open to other options that don't require going back to nested selectors.

Other changes of note from the old `.euiPopover__panel`

I've kept the naming of the component to be .euiPopover__panel instead of .euiPopoverPanel mainly to reduce breaks.
I've remove the old .euiPopover__panel-isOpen class in favor of using a data attribute that exists or doesn't called data-popover-open.

[New] EuiPopoverArrow (internal only)

This new component mainly handles the styling based on arrowPosition. The styles for this element used to be a complicated set of multiple pseudo elements, borders, and clip-paths. I've reduced this complexity to a single :before element with borders to create the triangle and then using the filter: drop-shadow() method of creating the shadow on the whole panel. Using filter takes into account the actual outline of an element where as box-shadow is only ever square.

Note
There is a slight difference between the render of the two methods even with the exact same values. This difference isn't enough to consider a whole different set of values for the filter method.

[New] `property` option for `euiShadowMedium()`

In order to quickly support the same values for this shadow size in both methods, I just added the property as an optional key in the optional parameter.

[New] JS mixin for `euiFormMaxWidth`

I needed this in order to re-use the variable in JS. I decided not to put it in the global theme at least for now and push the decision down the road on how we want to handle component-based re-usable variables.

[New] `logicalSizeCSS(width, height)`

For quickly creating width and height values.

[Update] Increased the opacity of shadow color in dark mode

It's always been pretty hard to see the outlines of panels with just a simple shadow, this helps a little bit, but I think we can follow up at some point with something even better.

[Conversion] EuiPopover & parts

EuiPopover: Base styles are pretty simple. Moves the popover panel and arrow to external components.
EuiInputPopover: This simply was ensuring the popover wrapper adhered to the same form width sizing by applying the new variable (but only when fullWidth=false).
EuiPopoverFooter & EuiPopoverTitle: See EuiPopoverPanel section above for inherited padding details. Otherwise nothing has changed except fo removing the padding classes.

Checklist

Inherited panel padding/margin

…lay` values

kibanamachine · 2022-06-16T18:50:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5977/

kibanamachine · 2022-06-16T20:12:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5977/

cchaos · 2022-06-16T20:43:27Z

This PR is ready for review, though it looks like CI is having a bad day.

Also note to those who attended the CSS-in-JS weekly when I talked about converting EuiPOpover to HOC. After pulling out the arrow and panel elements to their own components, I no longer need access to EuiTheme directly in EuiPopover, so I was able to revert that change.

kibanamachine · 2022-06-16T21:53:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5977/

elizabetdev

Thanks, @chaos. 🎉

The shadow in dark mode looks much better. I tested in Chrome, Safari, Edge, and Firefox. LGTM!

I found some minor issues. Nothing special.

elizabetdev · 2022-06-20T17:16:40Z

src/components/popover/popover.tsx

-        <div className={classes} ref={popoverRef} {...rest}>
-          <div className={anchorClasses} ref={this.buttonRef}>
+        <div css={popoverStyles} className={classes} ref={popoverRef} {...rest}>
+          <div css={{ display }} className={anchorClasses} ref={this.buttonRef}>


This render as eui-docs-16vtueo-render. Should you create a new key euiPopover__anchor in src/components/popover/popover.styles.ts so that it renders like eui-docs-kp09r4-euiPopover__anchor?

const popoverAnchorStyles = [styles.euiPopover__anchor, { display }]; <div css={popoverAnchorStyles} className={anchorClasses} ref={this.buttonRef}>

I went back and forth with myself on this one because the only css applied to this element is the display styles which are passed directly so styles.euiPopover__anchor wouldn't contain anything and would just be repeating euiPopover__anchor which we have to keep as a static class name of simply .euiPopover__anchor anyway.

Basically keeping to our "pattern" here just adds extra unused cruft.

elizabetdev · 2022-06-20T17:18:52Z

src/components/popover/popover_footer.tsx

+    // If a paddingSize is not directly provided, inherit from the EuiPopoverPanel
+    paddingStyles[paddingSize || panelPadding],


That's a great idea! 💡

src/components/popover/popover.tsx

src/components/popover/popover_panel/popover_panel.tsx

kibanamachine · 2022-06-30T16:28:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5977/

thompsongl · 2022-06-30T16:40:27Z

The does not focus anything if 'initialFocus' is false cypress test is failing consistently on CI and locally. Not sure what the cause is at the moment.

thompsongl · 2022-06-30T18:21:24Z

The does not focus anything if 'initialFocus' is false cypress test is failing consistently on CI and locally. Not sure what the cause is at the moment.

Still unsure of the cause, but almost more confused now because 1) initialFocus=false probably shouldn't have been working as expected previously and 2) I don't know why changing styling affected any focus behavior.

initialFocus is reset and allows EuiFocusTrap to set initial focus on the panel

eui/src/components/popover/popover.tsx

Lines 744 to 761 in b63fbce

    
           if (!this.state.suppressingPopover && (isOpen || this.state.isClosing)) { 
        
             let tabIndex = tabIndexProp; 
        
             let initialFocus; 
        
             let ariaDescribedby; 
        
             let ariaLive: HTMLAttributes<any>['aria-live']; 
        
             const panelAriaModal = panelProps?.hasOwnProperty('aria-modal') 
        
               ? panelProps['aria-modal'] 
        
               : 'true'; 
        
             const panelRole = panelProps?.hasOwnProperty('role') 
        
               ? panelProps.role 
        
               : 'dialog'; 
        
             if (ownFocus || panelAriaModal !== 'true') { 
        
               tabIndex = tabIndexProp ?? 0; 
        
               ariaLive = 'off'; 
        
               initialFocus = () => this.panel!;

Potentially some transition timing changed and "fixed" a race condition with setting focus

@constancecchen Does anything here sound familiar with your recent focus fighting work?

cee-chen · 2022-07-11T19:49:02Z

@thompsongl super apologies for taking so long to get back to this - bizarrely enough does not focus anything if 'initialFocus' is false is passing for me locally without issue. Going to merge in latest main and see what CI says

EDIT: To add even more to the weirdness, this passed locally for me when I ran yarn test-cypress-dev --skip-css but not after I ran it without the --skip-css command 💀

kibanamachine · 2022-07-11T20:15:43Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5977/

cee-chen · 2022-07-11T21:54:41Z

Potentially some transition timing changed and "fixed" a race condition with setting focus

This is the only rational explanation I can think of - Emotion is causing a few more rerender loops which is somehow triggering this change.

FWIW, I'm testing this locally with the let initialFocus; and initialFocus = () => this.panel!; completely removed and there's no change to the test failing. The only way I can get the test to pass as it was before (with focus remaining on the toggling button) is to completely remove the wrapping <EuiFocusTrap> 😖

I think what's happening is actually due to react-focus-on's autoFocus behavior. If I set EuiFocusTrap's autoFocus={false} when initialFocus={false} - focus is removed completely from the document. This is actually technically 'correct' behavior if a focus trap is enabled on the popover - because the toggling button is outside the popover, focus actually shouldn't have remained on it.

I think the best way forward frankly (after #5784) is to remove initialFocus={false} as an option at all. It no longer really makes sense after its first implementation in #4768, since false/focusing the panel is basically the default at this point.

~~@thompsongl would you be cool with me pushing that change up to this PR?~~ Actually it makes more sense to have this fix be a separate PR, IMO. I'll open something up here shortly

kibanamachine · 2022-07-20T19:47:37Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5977/

kibanamachine · 2022-07-20T20:26:38Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5977/

thompsongl

LGTM! The change to initialFocus makes it so that all functionality appears unchanged.

thompsongl · 2022-06-30T15:51:49Z

src/components/combo_box/combo_box_options_list/combo_box_options_list.tsx

-    const classes = classNames(
-      'euiComboBoxOptionsList',
-      'euiPopover__panel',
-      'euiPopover__panel-isAttached',
-      'euiPopover__panel-noArrow',
-      'euiPopover__panel-isOpen',
-      `euiPopover__panel--${position}`
-    );


Excellent refactor!

* Upgrade to v62.0.3 * Update EUI i18n tokens * Update html string snapshots - Emotion CSS hash changed * [EuiIcon] Update instances of `keyboardShortcut` icons to `keyboard` * [EuiErrorBoundary] Update snapshots from Emotion conversion * [EuiImage] Update snapshots, tests, and CSS to account for Emotion conversion * [EuiImage][RTL] Fix test failures caused by EuiImage changes * [EuiCommentList] Deprecate EuiCommentProps.type * [EuiCommentList] Rename `timelineIcon` prop to `timelineAvatar` - see elastic/eui#6071 * [EuiCommentList] Fix selectors deprecated by Emotion conversion * [EuiPopover][EuiCommentEvent][Enzyme] Fix mounted test failures caused by Emotion conversions - Mounting displays the Emotion wrapper with the data-test-subj on them - we need to specify the output div renders in order for text assertions to be correct * [EuiPopover] Deprecate `initialFocus={false}` as an option see elastic/eui#6044 * [EuiPopover] Rename `display=inlineBlock` to `inline-block` - see elastic/eui#5977 * [EuiPopover] Update snapshots from Emotion conversion * [EuiPopover] Replace deprecated `.euiPopover__panel-isOpen` class with new `[data-popover-open]` attribute * [EuiPopover][RTL] Fix test failures caused by not waiting for EuiPopover animation/transition * Skip failing a11y tests - test w/ similar error already skipped in another test above - requires closing the popover for next test to pass - not sure why delete action is no longer available * Fix failing Security Cypress tests * Attempt to squash flaky FTR tests around Add Filter popover Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Jonathan Budzenski <[email protected]>

cchaos added 18 commits June 14, 2022 08:58

Move Amsterdam Sass overrides to src/

9da3c20

[EuiPopoverFooter] Converted except for…

8affcf0

Inherited panel padding/margin

[EuiPopoverTitle] Converted except for…

85b3146

Inherited panel padding/margin

Removed Sass mixin euiPopoverTitle

3d6b35c

Added [EuiPopoverArrow] and simplified styles

e1ff004

[EuiPopover] BREAKING Changed display type to accept real CSS `disp…

3c825fd

…lay` values

Fix grid tests

568f6c5

WIP: Converted base EuiPaopover and panel styles

479d55e

New EuiPopoverPanel for internal use

9fec434

Skip test & fix props

612aa20

Fixed EuiTour’s styles

a1dd897

Undo HOC by moving panel styles to it’s own folder

65ad54b

Created a EuiPopoverPanelContext for passing through paddingSize

52d68f6

[EuiInputPopover] Converted by applying max-width directly in component

653cdc8

Converted to logical properties

3281c02

Merge remote-tracking branch origin/main into emotion/popover

9825f94

Convert some docs files to TS

b381d30

Increase opacity of shadows in DARK mode

4e3029f

cchaos added 2 commits June 16, 2022 15:36

Cleanup

8dc669d

cl

ebc20a9

cchaos marked this pull request as ready for review June 16, 2022 20:42

Mostly fix class name output

4d35b81

cchaos mentioned this pull request Jun 16, 2022

[Meta][CSS-in-JS] Component conversions #5685

Closed

49 tasks

elizabetdev self-requested a review June 20, 2022 16:17

elizabetdev approved these changes Jun 20, 2022

View reviewed changes

elizabetdev added the breaking change label Jun 20, 2022

Merge branch 'main' into emotion/popover

ddd912a

cee-chen mentioned this pull request Jul 11, 2022

[EuiPopover] Removed false as an option for initialFocus, removed unnecessary focus logic #6044

Merged

15 tasks

Merge branch 'main' into emotion/popover

192fc01

snapshot updates

b713470

thompsongl approved these changes Jul 20, 2022

View reviewed changes

thompsongl enabled auto-merge (squash) July 20, 2022 20:38

thompsongl merged commit b86ee8c into elastic:main Jul 20, 2022

cee-chen mentioned this pull request Jul 27, 2022

Upgrade EUI to v62.0.3 elastic/kibana#136865

Closed

cee-chen mentioned this pull request Aug 9, 2022

Upgrade EUI to v62.0.3 elastic/kibana#138351

Merged

breehall mentioned this pull request Aug 17, 2022

[EuiText] Updated euiTextContrainedMaxWidth Value #6146

Closed

4 tasks

cee-chen mentioned this pull request Aug 23, 2022

[DataGrid] Cell expansion missing border in Safari #6151

Closed

cee-chen mentioned this pull request Sep 21, 2023

[EuiPopover][EuiInputPopover] Misc fixes and CSS cleanup #7211

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiPopover and parts] Converted to Emotion #5977

[EuiPopover and parts] Converted to Emotion #5977

cchaos commented Jun 16, 2022 •

edited

Loading

kibanamachine commented Jun 16, 2022

kibanamachine commented Jun 16, 2022

cchaos commented Jun 16, 2022

kibanamachine commented Jun 16, 2022

elizabetdev left a comment

elizabetdev Jun 20, 2022

cchaos Jun 21, 2022

elizabetdev Jun 20, 2022

kibanamachine commented Jun 30, 2022

thompsongl commented Jun 30, 2022 •

edited

Loading

thompsongl commented Jun 30, 2022

cee-chen commented Jul 11, 2022 •

edited

Loading

kibanamachine commented Jul 11, 2022

cee-chen commented Jul 11, 2022 •

edited

Loading

kibanamachine commented Jul 20, 2022

kibanamachine commented Jul 20, 2022

thompsongl left a comment

thompsongl Jun 30, 2022

		// If a paddingSize is not directly provided, inherit from the EuiPopoverPanel
		paddingStyles[paddingSize \|\| panelPadding],

[EuiPopover and parts] Converted to Emotion #5977

[EuiPopover and parts] Converted to Emotion #5977

Conversation

cchaos commented Jun 16, 2022 • edited Loading

[Breaking] EuiPopover's display prop now accepts real CSS display values

[New] EuiPopoverPanel (internal only)

Added EuiPopoverPanelContext to pass through panelPadding

Other changes of note from the old .euiPopover__panel

[New] EuiPopoverArrow (internal only)

[New] property option for euiShadowMedium()

[New] JS mixin for euiFormMaxWidth

[New] logicalSizeCSS(width, height)

[Update] Increased the opacity of shadow color in dark mode

[Conversion] EuiPopover & parts

Checklist

kibanamachine commented Jun 16, 2022

kibanamachine commented Jun 16, 2022

cchaos commented Jun 16, 2022

kibanamachine commented Jun 16, 2022

elizabetdev left a comment

Choose a reason for hiding this comment

elizabetdev Jun 20, 2022

Choose a reason for hiding this comment

cchaos Jun 21, 2022

Choose a reason for hiding this comment

elizabetdev Jun 20, 2022

Choose a reason for hiding this comment

kibanamachine commented Jun 30, 2022

thompsongl commented Jun 30, 2022 • edited Loading

thompsongl commented Jun 30, 2022

cee-chen commented Jul 11, 2022 • edited Loading

kibanamachine commented Jul 11, 2022

cee-chen commented Jul 11, 2022 • edited Loading

kibanamachine commented Jul 20, 2022

kibanamachine commented Jul 20, 2022

thompsongl left a comment

Choose a reason for hiding this comment

thompsongl Jun 30, 2022

Choose a reason for hiding this comment

cchaos commented Jun 16, 2022 •

edited

Loading

[Breaking] EuiPopover's `display` prop now accepts real CSS `display` values

Added `EuiPopoverPanelContext` to pass through `panelPadding`

Other changes of note from the old `.euiPopover__panel`

[New] `property` option for `euiShadowMedium()`

[New] JS mixin for `euiFormMaxWidth`

[New] `logicalSizeCSS(width, height)`

thompsongl commented Jun 30, 2022 •

edited

Loading

cee-chen commented Jul 11, 2022 •

edited

Loading

cee-chen commented Jul 11, 2022 •

edited

Loading